Tooltip: forward and merge inner tooltip props correctly #57878
Conversation
ciampo
added
[Type] Bug
There was a problem hiding this comment.
I noticed that Tooltip was just recently connected to the Context System (#57202), specifically to deal with this nested tooltip problem.
Have we considered passing isNestedInTooltip via standard React context, rather than the wp-components Context System? Seems like it would make things simpler if that's possible. Also, if some random props added by the system can mess up the nested tooltip case, it might not be a good idea for Tooltip to be connected to the "public" Context System in the first place, since that could be a footgun for consumers that fail to consider the nested case.
ciampo
force-pushed
the
fix/tooltip-merge-inner-tooltip-props
branch
from
4c62e7e to
caeb992
Compare
That was how the first implementation of #57202 worked, but I seem to remember getting some feedback about using the context system. You comment makes sense though, as it would simplify the implementation and avoid the need for the custom I went ahead and applied the suggested changes, let me know what you think. |
There was a problem hiding this comment.
Here are some suggested tests to add in relation to this code change:
diff --git a/packages/components/src/tooltip/test/index.tsx b/packages/components/src/tooltip/test/index.tsx
index ed6f7b5f7b..9f7cba6eeb 100644
--- a/packages/components/src/tooltip/test/index.tsx
+++ b/packages/components/src/tooltip/test/index.tsx
@@ -102,6 +102,17 @@ describe( 'Tooltip', () => {
screen.queryByRole( 'button', { description: 'tooltip text' } )
).not.toBeInTheDocument();
} );
+
+ it( 'should not leak Tooltip props to the anchor element', () => {
+ render(
+ <Tooltip data-foo>
+ <button>Anchor</button>
+ </Tooltip>
+ );
+ expect(
+ screen.getByRole( 'button', { name: 'Anchor' } )
+ ).not.toHaveAttribute( 'data-foo' );
+ } );
} );
describe( 'keyboard focus', () => {
@@ -481,5 +492,33 @@ describe( 'Tooltip', () => {
).not.toBeInTheDocument()
);
} );
+
+ it( 'should not leak Tooltip classname to the anchor element', () => {
+ render(
+ <Tooltip>
+ <Tooltip>
+ <button>Anchor</button>
+ </Tooltip>
+ </Tooltip>
+ );
+ expect(
+ screen.getByRole( 'button', { name: 'Anchor' } )
+ ).not.toHaveClass( 'components-tooltip' );
+ } );
+
+ it( 'should not leak Tooltip props to the anchor element', () => {
+ render(
+ <Tooltip data-outer>
+ <Tooltip data-inner>
+ <button>Anchor</button>
+ </Tooltip>
+ </Tooltip>
+ );
+ const anchor = screen.getByRole( 'button', {
+ name: 'Anchor',
+ } );
+ expect( anchor ).not.toHaveAttribute( 'data-outer' );
+ expect( anchor ).not.toHaveAttribute( 'data-inner' );
+ } );
} );
} );I'm ok with merging this PR because it is an improvement over trunk (i.e. the classname leakage is fixed), but note that the inner Tooltip props in the nested case is still leaking onto the anchor.
Also, something I noticed in this test file is that it mostly tests Tooltip in combination with Button, which is fine when we're intentionally testing that specific coupling, but may not be ideal in terms of isolation when we consider the particular tooltip implementation that Button has under the hood. Perhaps something to keep in mind as we work on #56490!
Thank you, just pushed to the branch
The thing is, some props (added by ariakit) need to be forwarded to the anchor — event listeners, for example — but those are implementation details, and therefore it felt wrong to whitelist only certain props. Do you have any ideas in mind?
I also noticed that, and I'll probably open a separate PR where:
Update: opened #57975 |
Mmm, it should be fine most of the time, since all the explicit props of Tooltip are not forwarded as part of the rest props. And we never forwarded rest props until last week. Unless we start running into actual problems, we might be able to get away with just reverting this to That would set the correct expectations, at least. |
Sounds good, opened #58125 to implement your suggestions. |
What?
Following the work done in #57202 to support nesting
Tooltipcomponents in the React tree, this PR improves how forwarded props are merged / passed to thechildrenof the inner tooltip.Why?
As discussed in #56490 (comment), the approach from #57202 didn't fully consider how props are passed / forwarded / merged to the
childrenof nestedTooltipcomponents.This change is necessary for allowing nested
Tooltips and theirchildrento work as expected.How?
As suggested by @diegohaz , using
Ariakit.Roletakes care of merging props correctly.The other important part was to remove some of the props that are added by the internal context system, which are not intended for
childrenof nested tooltips.Testing Instructions
Play around with the "Nested Tooltip" storybook example by passing extra styles / classname / event handlers, make sure that they are forward correctly to the
childrenof the inner-mostTooltipcomponent.Make sure unit tests pass.